Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pos_ui_extension to migration for ui_extension #4162

Merged
merged 4 commits into from
Jul 5, 2024

Conversation

js-goupil
Copy link
Contributor

@js-goupil js-goupil commented Jul 4, 2024

WHY are these changes introduced?

We noticed that while testing a deploy for Stocky to the new ui extension spec, that the registrationUuid gets regenerated. This is problematic for smart grid tiles that are already existing, as they refer to these uuids in order to exist. If the registrationUuid no longer exists, then the tile will break

WHAT is this pull request doing?

Adds pos_ui_extension extension types to the migrator.

How to test your changes?

  1. I have a separate branch that has a test extension commited. Check this branch out in my spin instance.
  2. You'll need to create a new app by running shopify app init. You will also need an extension, but you can copy this folder and toss it into your new app. You can just copy this folder
    sandbox-170.zip
    into your local spin instance. Make sure you run npm install to install the dependencies.
  3. Deploy the extension by doing pnpm shopify app deploy --path {path to your app} --verbose. This should deploy the extension in the old spec.
  4. For convenience purposes, attach the app to the existing Extension Spin app, since it's already published on shop 1.
  5. Go to graphQL for shop-1, and query with the specificationIdentifier: "pos_ui_extension" to make sure that it's there. Take note of the registrationUuid.
  6. Change the toml file to the following:
name = "sandbox-170"
type = "ui_extension"
handle = "sandbox-170"
description = "sandbox-170"
api_version = "2024-04"

[[extension_points]]
target = "pos.home.tile.render"
module = "./src/index.tsx"

  1. Run pnpm build and pnpm shopify app build.
  2. Run pnpm shopify app deploy --path {path to your app} --verbose again. This time you should get a prompt to migrate your extension. Hit yes.
  3. This should run succesfully, and prompt you to deploy a new version.
  4. Once that's done, go to graphql for shop-1 and query with the specificationIdentifier: "ui_extension". You should see your extension there with the SAME registrationUuid.

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

This comment has been minimized.

Copy link

@rtymchyk rtymchyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this was used in the past & reads like it should do the right thing, so LGTM. Thank you!

Copy link
Member

@vividviolet vividviolet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm CI failing could be a red herring. Can you update the tests and try to re-run locally? We need a new test for pos_ui_extension

Copy link
Contributor

github-actions bot commented Jul 4, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
72.49% (-0% 🔻)
7547/10411
🟡 Branches
69.25% (+0.01% 🔼)
3718/5369
🟡 Functions
71.44% (-0.03% 🔻)
1996/2794
🟡 Lines
72.82% (+0% 🔼)
7127/9787
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🔴
... / prompts.ts
31.58% (-3.72% 🔻)
0%
30% (-3.33% 🔻)
28.57% (-2.2% 🔻)

Test suite run success

1735 tests passing in 800 suites.

Report generated by 🧪jest coverage report action from cb06f10

@js-goupil
Copy link
Contributor Author

@vividviolet good call out, turns out the issue was fixed by rebasing with main. Did add a test case though

Copy link
Member

@vividviolet vividviolet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. We should only merge after the Shopify PR gets shipped. Can you put up a PR? I'll top hat end-to-end with the other PR

@js-goupil
Copy link
Contributor Author

js-goupil commented Jul 4, 2024

@vividviolet yup, already had one in draft, but marked it as ready for review https://github.com/Shopify/shopify/pull/518353

Also not sure if it helps, but my spin instance linked above has both these cli changes and the core one

@vividviolet
Copy link
Member

ps, but my spin instance linked above has both these cli changes and the core one

I don't think I can point to your spin unfortunately so I have to check out your branches on my own instance

@isaacroldan
Copy link
Contributor

We have a release next week so it'll be ideal to have this merged today, let me know how the tophat goes @vividviolet 🙏

@vividviolet
Copy link
Member

/snapit

1 similar comment
@isaacroldan
Copy link
Contributor

/snapit

Copy link
Contributor

github-actions bot commented Jul 5, 2024

🫰✨ Thanks @isaacroldan! Your snapshot has been published to npm.

Test the snapshot by installing the package globally:

pnpm i -g @shopify/cli@0.0.0-snapshot-20240705142219

@js-goupil
Copy link
Contributor Author

/snapit

I had no idea this was a thing, that's cool as hell

Copy link
Member

@vividviolet vividviolet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested end-to-end - everything looks good. Since you're making changes to the migrator, do you mind updating the message? It's a bit confusing because it doesn't say what we're migrating to:
image

Maybe it should be

Your "sandbox-170" configuration has been updated to a UI extension and needs to be migrated. Migrating gives you access to new features and won't impact the end user experience. All previous extension versions will reflect this change.

And then the prompt should be "Yes, confirm migration from "pos_ui_extension" to "ui_extension"?

@isaacroldan
Copy link
Contributor

Agree on Trish's proposal 👌
Also, please run pnpm changeset add to add a patch changeset in this PR

'@shopify/cli': minor
---

Added extension type pos_ui_extension to the ui_extension migration process
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -41,13 +41,15 @@ export async function extensionMigrationPrompt(
.map((name) => `"${name}"`)
.join(', ')

const migrationEndType = toMigrate.map(({local}) => `"${local.type}"`).join(', ')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Followed the same structure as the other variables in the prompt, but oddly enough this always ends up being one value in the toMigrate, even if there are two extensions to be migrated. Then if you deploy again, it will go to the second extension that needs to be migrated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isaacroldan any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean you have 2 local ui_extensions and 2 remote pos_ui_extension but only 1 in the toMigrate?

The code at getUIExtensionsToMigrate is pretty simple, it just finds matching extensions locally and remotely, you can debug that method and see why it might not match a specific extension.

Also, specifically about this line of code, you are not filtering for duplicated types here, check how uniqueMigrationTypes does it just above.

renderInfo({
headline: "Extension migrations can't be undone.",
body: `Your ${migrationNames} configuration has been updated. Migrating gives you access to new features and won't impact the end user experience. All previous extension versions will reflect this change.`,
})

const confirmMessage = includeRemoteType
? `Yes, confirm migration from ${uniqueMigrationTypes}`
? `Yes, confirm migration from ${uniqueMigrationTypes} to ${migrationEndType}`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-07-05 at 1 14 04 PM

@js-goupil js-goupil added this pull request to the merge queue Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants